feat: shareable URLs for library components and searches [FC-0076] #1575

pomegranited · 2024-12-18T07:35:27Z

Description

This PR adds new routes and URL parameters to use when viewing and performing searches on library components. These changes allow these pages to be bookmarked or shared by copy/pasting the browser's current URL.

No changes were made to the UI.

Use cases covered:

As an author working with content libraries, I want to easily share any component in a library with other people on my team, by copying the URL from my browser and sending it to them.
As an author working with content libraries, I want to easily share any search results with other people on my team, by copying the URL from my browser and sending it to them.
As an author working with content libraries, I want to bookmark a search in my browser and return to it at any time, with the same filters and keywords applied.
As an author of a content library with public read access, I want to easily share any component in a library with any authors on the same Open edX instance, by copying the URL from my browser and sending it to them.
As an author of a content library, I want to easily share a library's "Manage Team" page with other people on my team by copying the URL from my browser and sending it to them.
As an author working with content libraries, I want to easily share any selected sidebar tab with other people on my team, by copying the URL from my browser and sending it to them.

Supporting information

See #1499
Private-ref: FAL-3984

Testing instructions

Code review: Recommend going commit-by-commit, since this is a pretty big change.

This change involved some route additions and changes, and so to manually test them, please refresh your browser page after each step, and ensure you see exactly the same page -- including selected tabs, sidebar, and sidebar tabs. Please also check that using the back/forth buttons navigate smoothly between pages as expected, but that changing the library search parameters doesn't append to the browser history.

Navigate to /libraries and create or select an existing library from the list.
"All Content" should still be the default tab you see.
Create a few components, collections, and add some components to some collections.
Note that the component picker still works as expected.
Navigate back to "All Content".
Select a component from the list.
Note that the URL changes to /library/:libraryId/component/:componentId, and the component is visible in the sidebar.
Add or select a different component from the list.
Note that the URL changes to reflect the new componentId.
Add or select a collection from the list.
Note that the URL changes to /library/:libraryId/:collectionId, and the collection is visible in the sidebar.
Add or select a new collection from the list.
Note that the URL changes to reflect the new collectionId.
Change your sidebar tab to something other than the initial Preview, then select a different component from the list.
Note that the sidebar tab is preserved (where possible) when navigating between components or collections.
Clicking Library Info should reset the sidebar back to the Library.
Change to the Components and Collections tabs.
Note that you remain in the selected tab when switching between selected components/collections.
Select a Collection, and either click the Open button in its sidebar or click the Collection card again to open it.
Note that the URL changes to /library/:libraryId/collection/:collectionId, and the collection is visible in the sidebar.
Select a component within the collection.
Note that the URL changes to /library/:libraryId/collection/:collectionId/:componentId, and the component is visible in the sidebar.
Change your sidebar tab to something other than the initial Preview, then select a different component from the list.
Note that the sidebar tab is preserved (where possible) when navigating between components.
Clicking Collection Info should reset the sidebar back to the Collection.
Use the search bar keywords to search for components from any tab or page.
Note that your keywords are stored in a q query string parameter, and preserved on page refresh.
Repeat for the other search filters and sort options.
Note that these are store in the query string, and cleared when de-selecting or clicking Clear Filters.
Navigate back to the Library sidebar, and click Manage Access.
Note that ?sa=manage-team is the query string, and refreshing the page re-opens the Library Team modal.
Using the Component card menu, select Add to Collection.
Note that ?sa=jump-to-add-collections is in the query string, and this is preserved when you navigate between components.
Clicking "Confirm" or "Cancel" removes this parameter and closes the collection manager.
Check that the Problem Bank and Library Content pickers still work as expected.

Author Notes & Concerns

When feat: allow filtering library by publish status #1570 merges, we'll need to apply the same "URL search params" treatment to the "Publish Status" filter that we've done with the other filters.
I chose not to store whether the sidebar was open or closed in the query string, because the sidebar reopens itself almost everywhere in library authoring, so it seemed unnecessary.
I left some TODOs in the SearchManager code about sanitizing parameters, because sanitising user input is good practice. But I'm not sure how best to sanitise things like search keywords?
I ran into a really tricky bug when running clearFilters on multiple search filters that are now using the useStateWithUrlSearchParam hook -- more details in this comment. Advice or alternatives welcome!
I tried to add tests to validate the navigation/routes used in this PR, but path changes aren't visible with the <MemoryRouter> used with testUtils, and none of the other available <Router> classes would let me set the base libraryId path. Advice welcome :)

to support optional parameters in test paths

openedx-webhooks · 2024-12-18T07:35:31Z

Thanks for the pull request, @pomegranited!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

…e test route.

codecov · 2024-12-19T01:29:56Z

Codecov Report

Attention: Patch coverage is 96.73203% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.98%. Comparing base (dc0ba6a) to head (39fd745).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/search-manager/hooks.ts	94.00%	3 Missing ⚠️
src/library-authoring/routes.ts	95.65%	2 Missing ⚠️
...c/library-authoring/collections/CollectionInfo.tsx	92.30%	1 Missing ⚠️
...library-authoring/component-info/ComponentInfo.tsx	83.33%	1 Missing ⚠️
...library-authoring/components/BaseComponentCard.tsx	0.00%	1 Missing ⚠️
src/library-authoring/library-info/LibraryInfo.tsx	88.88%	1 Missing ⚠️
src/search-manager/FilterByBlockType.tsx	96.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1575      +/-   ##
==========================================
+ Coverage   92.97%   92.98%   +0.01%     
==========================================
  Files        1075     1077       +2     
  Lines       21205    21353     +148     
  Branches     4562     4542      -20     
==========================================
+ Hits        19715    19855     +140     
- Misses       1424     1432       +8     
  Partials       66       66

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ble-urls

* Restructures LibraryLayout so that LibraryContext can useParams() to initialize its componentId/collectionId instead of having to parse complicated route strings. Initialization-from-URL can be disabled for the content pickers by passing skipUrlUpdate to the LibraryContext -- which is needed by the component picker. * Clicking/selecting a ComponentCard/CollectionCard navigates to an appropriate component/collection route given the current page. * Adds useLibraryRoutes() hook so components can easily navigate to the best available route without having to know the route strings or maintain search params. * Moves ContentType declaration to the new routes.ts to avoid circular imports. * Renames openInfoSidebar to openLibrarySidebar, so that openInfoSidebar can be used to open the best sidebar for a given library/component/collection.

This triggers a navigate() call when the searchbox changes, which resets the query counts in testing. So had to reduce the expected counts by 1.

Related changes: * adds "manage-team" sidebar action to trigger LibraryInfo's "Manage Team" modal * moves useStateWithUrlSearchParam up to hooks.ts so it can be used by search-manager and library-authoring * splits sidebarCurrentTab and additionalAction out of SidebarComponentInfo -- they're now managed independently as url parameters. This also simplifies setSidebarComponentInfo: we can simply set the new id + key, and so there's no need to merge the ...prev state and new state. * shortens some sidebar property names: sidebarCurrentTab => sidebarTab sidebarAdditionalAction => sidebarAction * test: Tab changes now trigger a navigate() call, which invalidates the within(sidebar) element in the tests. So using screen instead.

bradenmacdonald

Ooops, queued up these comments when I took an early look at this PR and forgot to submit. Not sure if they're still useful.

src/hooks.ts

bradenmacdonald · 2024-12-18T18:39:28Z

src/library-authoring/LibraryLayout.tsx

+    <Routes>
+      <Route
+        path={ROUTES.COMPONENTS}
+        element={context(<LibraryAuthoringPage />)}
+      />
+      <Route
+        path={ROUTES.COLLECTIONS}
+        element={context(<LibraryAuthoringPage />)}
+      />
+      <Route
+        path={ROUTES.COMPONENT}
+        element={context(<LibraryAuthoringPage />)}
+      />
+      <Route
+        path={ROUTES.COLLECTION}
+        element={context(<LibraryCollectionPage />)}
+      />
+      <Route
+        path={ROUTES.HOME}
+        element={context(<LibraryAuthoringPage />)}
+      />
+    </Routes>


Is this entire <Routes>...</Routes> worth having instead of just putting <LibraryAuthoringPage /> ? I guess it will show a 404 error if none of the routes match? Or does it?

The advantage of spelling out all of these routes is that it's now very easy to get the :componentId / :collectionId from useParams() and use these as the defaults in LibraryProvider.

I also had to wrap the page element in that context() wrapper so that the routes would be available when LibraryProvider is created -- previously <LibraryLayout> wrapped <LibraryProvider> around the <Routes>, and so <LibraryProvider> couldn't use them.

src/library-authoring/routes.ts

Related features: 1. We can filter on more than one tag, so the search field needs to store an Array of values which still need to be sanitised. This change adds useListHelpers to assist with the parsing and validating of an Array of Typed values. 2. SearchManager takes a skipUrlUpdate property which should disable using search params for state variables. Our "sort" parameters respected this, but none of the other search parameters did. So this change also adds a wrapper hook called useStateOrUrlSearchParam that handles this switch cleanly. This feature also revealed two bugs fixed in useStateWithUrlSearchParam: 1. When the returnSetter is called with a function instead of a simple value, we need to pass in previous returnValue to the function so it can generate the new value. 2. When the returnSetter is called multiple times by a single callback (like with clearFilters), the latest changes to the UrlSearchParams weren't showing up. To fix this, we had to use the location.search string as the "latest" previous url search, not the prevParams passed into setSearchParams, because these params may not have the latest updates.

Combine blockTypesFilter + problemTypesFilter into a single state variable called typesFilter, and store selected block + problem types in the url search parameters. These two states are heavily interdependent, and so the FilterByBlockType code was quite complicated. Then storing these states as url search params caused re-renders that disrupted the delicate dependencies between these two states. This change stores both these type lists using a single TypesFilterData class to simplify the code and avoid bugs when updating their states.

navinkarkera

@pomegranited Everything works nicely! Great work!.

Just found a small bug: On collections page, select any collection so that its sidebar appears (this appends st=manage to query params), now click on Library Info button and then on Manage Access button. This opens up Library Team modal and adds sa=manage-team query param to url. On refreshing page, modal is not displayed, most probably due to presence of st=manage query param in url. We should remove it on clicking Library info button or display of library sidebar.

Using the Component card menu, select Add to Collection. Note that ?sa=jump-to-add-collections is in the query string, and this is preserved when you navigate between components.

Should we also add this parameter when author clicks on Add to Collection button in the component sidebar?

src/library-authoring/LibraryAuthoringPage.tsx

src/hooks.ts

…ble-urls

clearing out the selected component/collection.

when editing component collections.

on library authoring page.

using typescript function overloading. Multiple values in the search params are provided by UrlSearchParams.getAll(). Also: * removes the useListHelpers hook added in previous commits -- it is no longer needed. * moves the useStateOrUrlSearchParam hook and TypesFilterData class to a separate search-manager/hooks.ts * sanitizes tag search parameters using oel RESERVED_TAG_CHARS * uses getBlockType to sanitize usage key search parameters

Re-implements the functionality added by openedx#1576 which was broken when storing types in search params. Had to use a different approach to avoid "insecure operation" errors from React. Also adds tests.

need to wrap the ManageCollections element in SidebarProvider.

Since search filters are now stored in the URL, they can leak between tests.

navinkarkera

@pomegranited This looks and works great. Thank you for taking this up. 💯

I tested this: (Played around with different pages and options and verified change in URL.)
I read through the code
I checked for accessibility issues
Includes documentation

navinkarkera · 2025-01-02T06:12:21Z

src/search-manager/hooks.ts

+  static #sep1 = ','; // separates the individual types
+
+  static #sep2 = '|'; // separates the block types from the problem types


@pomegranited NIT: we can replace these separators with browser's ability to parse multiple query params with same name. But I don't wish to block this as we can update it in a separate PR (if we want to) in the future. Currently, I cannot find any real problems with this approach and it works perfectly.

Hi @navinkarkera -- I'd love to do this, but I'm not sure how, since there's two different "types" that we're tracking here -- block and problem types -- I can't just use a simple ?type=html&type=checkboxes&... solution. I had to merge them into a single object because they're so entwined (see FilterByBlockType), and so when I then tried to manage their state with URLSearchParams, I got all kinds of terrible react errors.

I'm open to ideas though -- I just can't see a clear way to doing this.

@pomegranited I was thinking about using a different param name for problem types for example:
?type=html&type=checkboxes&problemType=multiplechoiceresponse&problemType=optionresponse
But if it is causing issues with react and requires a lot of time to implement, we can skip it for now.

It won't take much time to split these params -- it's just a reversion back to where they were before. And it's possible the stuff I did to workaround the URLSearchParams state issue has fixed the react problems I was having when I combined them.

@ChrisChV what do you think?

I think Jill's solution is fine for now. I'm not one to risk errors in reverting when they've already been fixed. In any other case, it could be done later. What do you think?

ChrisChV

Looks good 👍 Only one nit about the coverage

ChrisChV · 2025-01-08T19:29:09Z

src/library-authoring/routes.ts

@pomegranited I think we should get to 100% test coverage for this file. do you think it's possible?

Sure thing -- I'll write some tests.

Couldn't get routes.ts coverage to 100% (because of MemoryRouter), but I could do 95%.

ChrisChV · 2025-01-08T21:05:57Z

src/search-manager/hooks.ts

+  static #sep1 = ','; // separates the individual types
+
+  static #sep2 = '|'; // separates the block types from the problem types


I think Jill's solution is fine for now. I'm not one to risk errors in reverting when they've already been fixed. In any other case, it could be done later. What do you think?

fix: use generatePath in testUtils

aa25827

to support optional parameters in test paths

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 18, 2024

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Dec 18, 2024

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from 8fdbe9b to aa25827 Compare December 19, 2024 01:11

fix: test uses contentId=undefined, so made this param optional in th…

a4728e1

…e test route.

Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…

a6465cb

…ble-urls

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from 5fad981 to 18b4e2d Compare December 19, 2024 03:41

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from 18b4e2d to ce4c27e Compare December 19, 2024 03:56

feat: store search keywords in query string

fc2f467

This triggers a navigate() call when the searchbox changes, which resets the query counts in testing. So had to reduce the expected counts by 1.

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from d202a8d to b1fe66e Compare December 19, 2024 04:45

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from b1fe66e to f6b46c4 Compare December 19, 2024 05:14

bradenmacdonald reviewed Dec 19, 2024

View reviewed changes

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from fa8783d to f6bf086 Compare December 20, 2024 02:39

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from f6bf086 to 37370d2 Compare December 20, 2024 13:16

pomegranited added 2 commits December 20, 2024 23:59

docs: adds more comments to the library authoring routes

a3ee610

pomegranited marked this pull request as ready for review December 20, 2024 14:45

pomegranited requested a review from a team as a code owner December 20, 2024 14:45

pomegranited requested a review from navinkarkera December 20, 2024 14:46

navinkarkera suggested changes Dec 23, 2024

View reviewed changes

src/library-authoring/LibraryAuthoringPage.tsx Outdated Show resolved Hide resolved

src/hooks.ts Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…

9d3e049

…ble-urls

pomegranited force-pushed the jill/fal-3984-sharable-urls branch from c59f109 to 9d3e049 Compare December 30, 2024 07:05

pomegranited added 3 commits December 30, 2024 17:36

fix: clicking "Library Info" navigates to the Library URL

5054020

clearing out the selected component/collection.

fix: use sidebarAction instead of managing separate state

88dfb0e

when editing component collections.

refactor: use getActiveKey to initialize the tab state

9ae67f5

on library authoring page.

pomegranited added 5 commits December 31, 2024 16:22

fix: disable filter by type on collections tab

b4c70ca

Re-implements the functionality added by openedx#1576 which was broken when storing types in search params. Had to use a different approach to avoid "insecure operation" errors from React. Also adds tests.

test: fixes broken tests

ced360e

need to wrap the ManageCollections element in SidebarProvider.

test: clear search filters in between tests

d060113

Since search filters are now stored in the URL, they can leak between tests.

test: split long-running "can filter by capa problem type" test into two

4c1855d

navinkarkera approved these changes Jan 2, 2025

View reviewed changes

ChrisChV approved these changes Jan 8, 2025

View reviewed changes

test: adds tests for library authoring routes.ts

39fd745

ChrisChV approved these changes Jan 10, 2025

View reviewed changes

ChrisChV merged commit 811be22 into openedx:master Jan 10, 2025
7 checks passed

ChrisChV deleted the jill/fal-3984-sharable-urls branch January 10, 2025 15:36

pomegranited mentioned this pull request Jan 16, 2025

fix: a11y and performance issues with library authoring navigation [FC-0076] #1593

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: shareable URLs for library components and searches [FC-0076] #1575

feat: shareable URLs for library components and searches [FC-0076] #1575

pomegranited commented Dec 18, 2024 •

edited

Loading

openedx-webhooks commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

bradenmacdonald left a comment

bradenmacdonald Dec 18, 2024

pomegranited Dec 20, 2024

navinkarkera left a comment

navinkarkera left a comment

navinkarkera Jan 2, 2025 •

edited

Loading

pomegranited Jan 2, 2025

navinkarkera Jan 3, 2025

pomegranited Jan 8, 2025

ChrisChV Jan 8, 2025

ChrisChV left a comment

ChrisChV Jan 8, 2025

pomegranited Jan 10, 2025

pomegranited Jan 10, 2025

ChrisChV Jan 8, 2025

		static #sep1 = ','; // separates the individual types

		static #sep2 = '\|'; // separates the block types from the problem types

feat: shareable URLs for library components and searches [FC-0076] #1575

feat: shareable URLs for library components and searches [FC-0076] #1575

Conversation

pomegranited commented Dec 18, 2024 • edited Loading

Description

Supporting information

Testing instructions

Author Notes & Concerns

openedx-webhooks commented Dec 18, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera left a comment

Choose a reason for hiding this comment

navinkarkera left a comment

Choose a reason for hiding this comment

navinkarkera Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited commented Dec 18, 2024 •

edited

Loading

openedx-webhooks commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

navinkarkera Jan 2, 2025 •

edited

Loading